-
Notifications
You must be signed in to change notification settings - Fork 29
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
factory: do not mount /lib/{modules,firmware} if there is a drivers tree #238
factory: do not mount /lib/{modules,firmware} if there is a drivers tree #238
Conversation
Draft for the moment, please do not merge - This will need to wait for snapd 2.63 to be in stable |
537cf31
to
1be1983
Compare
@valentindavid @Meulengracht this is ready for review now. |
1be1983
to
e1cd0ee
Compare
@alfonsosanchezbeato Could we have a reference of what PR from snapd will expect this? |
This change enables using (mounting) a kernel drivers tree when created by snapd. All the needed changes in snapd were already merged. The ways this works is:
Hopefully this answers your question. |
I suppose this was this one: canonical/snapd#13923 |
And canonical/snapd#13563 and possibly others. |
factory/usr/lib/core/extra-paths
Outdated
EOF | ||
|
||
# Mount /lib/{firmware,modules} only if there is no drivers tree created by snapd | ||
if [ ! -d /run/mnt/data/system-data/var/lib/snapd/kernel ]; then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What are the situation where this is not created? I think of 2, but I want to make sure I understand.
- When snapd is too old (will not be the case in uc26 anymore)
- It is the first boot, or factory reset
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could happen for old snapd or base < core24 so snapd does not create the drivers tree. Also on install mode or factory reset, when you have an ephemeral system. But for first boot after installation the drivers tree should be there (see canonical/snapd#13923).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BTW, does snapd generate /etc/systemd/system/usr-lib-modules.mount
? Then this already should have more priority than /run/systemd/generator/usr-lib-modules.mount
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, we do not generate that mount in /etc
.
In any case, as discussed it would be good to check for the path including the revision (/var/lib/snapd/kernel/<kernel_snap_name>/<snap_revision>/
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Out of curiosity, could you explain how the modules are found? Usually, they have to be placed in /lib/modules/$(uname -r)
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Modules are mounted by the systemd generator (https://github.com/snapcore/snapd/blob/master/cmd/snapd-generator/main.c) that now we include in core24.
Do not mount /lib/{modules,firmware} in case snapd created a drivers tree for our kernel snap, as in that case these mounts will be created by snapd-generator from the base or from the snapd deb package for hybrid systems. This situation is expected to happen only for 24+ systems.
e1cd0ee
to
96794bd
Compare
@valentindavid I now check for kernel snap name / revision, I think that should handle most corner case situations. And hopefully the change guarantees that you will have a drivers tree compatible with the loaded kernel, and any core24/hybrid 24.10 system has in theory the right generator for these mounts. I have tested the change for UC24 and hybrid in different scenarios, and seems to work as expected. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, based on the changes that use the rev directly this looks good
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks fine.
The tests-snapd tests seem to fail because no eth device is created in the VM. This looks as the same as #246 , so the issue does not seem directly related to the PR. |
dd83fe2
to
c621725
Compare
I'd like to set |
Do not mount /lib/{modules,firmware} in case snapd created a drivers
tree for our kernel snap, as in that case these mounts will be created
by snapd-generator from the base or from the snapd deb package for
hybrid systems. This situation is expected to happen only for UC24+.